Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented Apartment Image Frontend #359

Merged
merged 6 commits into from
May 1, 2024
Merged

Conversation

ggsawatyanon
Copy link
Contributor

Summary

This pull request is the first step towards styling apartment images and implementing a photo carousel.

  • Implemented consistent styling for apartment card images
  • Implemented new style for apartment header images
  • Implemented new mobile logic for apartment header (using ternary operations instead of returning an entirely different component when in mobile mode)
  • Implemented apartment photo carousel
  • Matched color of icon for apartment page and landlord page

Test Plan

  1. Consistent styling for apartment card images (between apartments with images and no images):
image
  1. Apartment header new style (gradient)
image Header when there are no images image
  1. Mobile display
image
  1. Photo carousel (accessed by clicking on the header)
image On mobile: image

Notes

@dti-github-bot
Copy link
Member

dti-github-bot commented Mar 24, 2024

[diff-counting] Significant lines: 238.

Copy link
Member

@kea-roy kea-roy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review

This pull request was nicely done! I appreciate your changes to Header.tsx for the Apartment and Landlord pages! The macro-level split between mobile and desktop layouts made it difficult to adjust every time a small change was made, but you made the code structure more concise by combining the two and using ternary operators to differentiate at the micro level. The photo carousel was also really well done. The modal design works perfectly on both desktop and mobile.

I would say it is nearly perfect, but I would like to point out a few things that could be improved for consistency and simplicity purposes.

  1. After removing parts of the code in Header.tsx, you can remove the imports that are no longer used to simplify the code, below is what eslint noticed:
src/components/Apartment/Header.tsx
  Line 6:3:    'Button' is defined but never used               @typescript-eslint/no-unused-vars
  Line 9:3:    'Avatar' is defined but never used               @typescript-eslint/no-unused-vars
  Line 192:5:  'btnSection' is assigned a value but never used  @typescript-eslint/no-unused-vars

src/components/Landlord/Header.tsx
  Line 10:3:   'Avatar' is defined but never used         @typescript-eslint/no-unused-vars
  Line 150:9:  'icon' is assigned a value but never used  @typescript-eslint/no-unused-vars

src/components/PhotoCarousel/PhotoCarousel.tsx
  Line 2:10:  'Modal' is defined but never used  @typescript-eslint/no-unused-vars
  1. On mobile, the marginLeft styling seems to incorrectly shift the header image, causing the header image to not fully extend to the edges of the screen. If you zoom in on the left, you can see a pixel gap between the edge of the screen and the image. This is a really small thing, but it could be nice to fix if you have the time. It could also have to do with the root paddingLeft and paddingRight, because when I disabled those in developer tools, it resolved the issue.
Screenshot 2024-04-09 at 18 30 42

Overall, these are extremely minor issues, so great work on this PR, it looks great!

Copy link
Contributor

@dan-ieljin dan-ieljin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean implementation. UI looks good.

@dan-ieljin dan-ieljin merged commit bc25d44 into main May 1, 2024
5 checks passed
@dan-ieljin dan-ieljin deleted the apartmentImageStyle branch May 1, 2024 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants